From a quick glance, the PR itself looks nice code-wise (even though function name could be shortened to make it look less bloated).

On the "game design" aspect, I'm not fond of the change because first person mode is way too powerful to be in logic imo.
The problem with FPM is that with a quite low skill ceiling, it pretty much gives access to everything, and a logic where you have access to almost everything means items are just placed anywhere without constraint and it just becomes "go everywhere and hope you get the item you need".

If it was harmless to the code, I think I'd just say "let's put it just in case people want to play with it", but it complexifies logic considerably in order to pretty much nullify it, which is not a good trade in my opinion.

Please note this is only my opinion and @evilwb will most likely have the final word on this, so this doesn't mean it won't get merged.
I can see there was work and dedication put into this, so I'm sorry to come down to this conclusion and I hope it doesn't ruin your motivation from contributing to the project in case it would not get merged.

These are valid concerns.
I also saw the complexity rising and I wonder how the logic functions will look like with more skips implemented.

Don't worry, this doesn't ruin any motivation towards trying to enhance the randomizer.
I'm already thinking about some tamer logic skips, if the FPM PR doesn't get in, so be it.

I feel apprehensive about adding something like this to the code base as it currently stands. This is not because this specific change is bad but is more of a big picture issue.

I can see two big categories of logic features on the horizon:

• combat difficulty logic
• glitch logic.

I think combat difficulty logic is more pressing out of the two. The change suggested above would likely be a part of a bigger glitch logic feature down the road. Unfortunately, I think there will need to be structural changes to the current code base to allow it to scale with the massive amount of conditional branches these advanced logic features will require. I haven't put a lot of thought into how we'd implement this yet but I think it will become spaghetti fast if not careful.

I changed the approach concerning the addition of logic.

I replaced the access_rule variable by 2 new ones:

• vanilla_requirements, which lists the requirements if following the intended route
• alternative_routes, which contains a list of rules and the requirements those conditions remove

Here's an example of how it looks:

YEEDIL_DEFEAT_MUTATED_PROTOPET = LocationData(
    None, "Yeedil: Defeat Mutated Protopet",
    vanilla_requirements=[can_hypnotize, can_swingshot, can_dynamo, can_infiltrate, can_electrolyze, can_improved_jump],
    alternative_routes=[
        AlternativeRoute(lambda options: options.allow_first_person_mode_glitch_locations >= FIRST_PERSON_HARD,
            [can_hypnotize, can_swingshot, can_dynamo, can_electrolyze, can_improved_jump]
        ),
        AlternativeRoute(lambda options: options.allow_first_person_mode_glitch_locations >= FIRST_PERSON_MEDIUM,
            [can_dynamo, can_improved_jump]
        ),
        AlternativeRoute(lambda options: options.allow_first_person_mode_glitch_locations >= FIRST_PERSON_EASY,
            [can_improved_jump]
        )
    ]
)

At generation, we take the content of vanilla_requirements and then remove any requirement that is in any AlternativeRoute satisfying the condition.
For the example above, if we set the FPM option to MEDIUM, we'd have can_hypnotize, can_swingshot, can_infiltrate and can_electrolyze

This way, I think we could more easily add logic to the checks.
In the event a requirement has to have 2 options or more enabled to be skipped, we can add an AlternativeRoute for that specific condition, but I don't know if there will be any need to.

That's an interesting approach, I like the idea to be able to condition routes in a well-constrained and enumeratable way.
Although, I find the current syntax to be sometimes a bit sub-optimal:

MAKTAR_PHOTO_BOOTH = LocationData(
    21, "Maktar: Photo Booth", 
    vanilla_requirements=[can_electrolyze],
    alternative_routes=[
        AlternativeRoute(lambda options: options.allow_first_person_mode_glitch_locations >= FIRST_PERSON_MEDIUM,
            [can_electrolyze],
            alternative_checks=[can_heli]
        )
    ]
)

I don't like the alternative_checks list, I feel like this is way too complex for what it does. If I understood well, it is for handling OR conditions? If that's the case, I'd prefer if it gave something like this:

MAKTAR_PHOTO_BOOTH = LocationData(
    21, "Maktar: Photo Booth", 
    vanilla_requirements=can_electrolyze,
    alternative_routes=[
        AlternativeRoute(lambda options: options.allow_first_person_mode_glitch_locations >= FIRST_PERSON_MEDIUM,
            lambda state, player: can_electrolyze(state, player) or can_heli(state, player)
        )
    ]
)

Overall, this is a step in the right direction into finding a way of improving how complex parameterized logic can be made as clear as possible, but I feel like we're not there yet 😉

I wanted the alternative_checks so that we can say "If we can use that route, we need that check instead".

I realize that the way I made it forces the alternative route in case we have alternative checks.
Indeed, I would need to look at how to better make alternative routes with alternative checks.

Maybe it's better to make vanilla_requirements and AlternativeRoute's 2nd parameter Callables instead of lists after all.
And enumerate the routes on generation.

My inclination would have been to go a different direction. I see two different routes to dealing with things like this and there's been similar situations with other parts of the code in the past. The routes are:

• A data driven approach with a generic but more complex function that processes well structured data where desired behavior is defined in data.
• A procedural approach with lots vary specific functions where the behavior is defined for each situation inside each function.

I'm always torn between which is better and suspect it depends on the problem. You've gone with the more data driven approach so far. The pros of the data driven approach are:

• More structured so it is hard to use it the wrong way
• Less code duplication
• Good separation between code and data

The Cons:

• Does not handle outlier/edge cases very well (less flexible)
• The more generic code required to handle it is often harder to reason about

The pro/cons are basically reversed for the procedural approach.

One other thing that's bothering me is how big Locations.py is getting. When we want to look at/edit logic, I think we should be able to do that mostly in Logic.py. My idea would be to keep Regions.py and Locations.py as simple as possible and do most the heavy lifting for logic in Logic.py. There could essentially be a function for each location in Logic.py like oozla_outside_megacorp_store_rule(state, player) -> bool. The LocationData could just point to the correct function for each location.

I think this offers a simple solution with lots of flexibility for each location with the main downside of some possible code duplication across functions. I'm not necessarily 100% attached to what I've suggested here and have probably overlooked some details but I think it is route that makes a lot of sense in this case. I'm am definitely open to feedback in this.

I was actually thinking the same thing this afternoon while browsing the code: Locations.py would benefit being smaller and have logic extracted to Logic.py.

I completely agree Locations.py is getting quite big for what it is and that logic should go into Logic.py.

I did as you suggested and created a function for each check that has requirements.

As you said, it offers more flexibility, which we might need in the future with more logic implemented.

I'm not quite satisfied with the way I'm doing alternative routes with different requirements. But as it is right now, it shouldn't be a problem.

Overall, very well done. The logic rules end up being easy to read/interpret and the rest of the code stays simple. The duplication in each function is a little annoying but definitely worth it.

Here's another question. Is there a meaningful benefit to having an option to enable first person mode instead of just having it always be enabled?

FPM is a single menu option which doesn't change anything beside switching between 3rd person and 1st person.
I don't know why a Rac2Options variable has been added, but I think we can remove it and have FPM always available

I would like to test generation on my dev machine after work as a sanity check and give @Dinopony a chance to respond. Assuming nothing else comes up, this should be good to merge soon.

I added it as an option to be able to keep it forbidden since it's pretty broken (e.g. in a racing context).
Honestly, option could be removed, in the worst case people can agree not to use it 👍

I was completely taken by text decoding & re-encoding and didn't take the time to review it.
I'd like to take a look tomorrow (in ~10h from now) and do a small review if it can wait 🙏

I think it makes sense to rename the option, especially after removing the other one.
I like the description you're suggesting, so I put it in both Rac2Options.py and Simplified Ratchet & Clank 2.yaml.

All good to me, I'll let @evilwb do a sanity check and merge the PR.
Thanks for that contribution 👍